Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(executor): added MsgUpsertSequencer consensus message #1120

Closed
wants to merge 30 commits into from

Conversation

keruch
Copy link
Contributor

@keruch keruch commented Oct 4, 2024

PR Standards

ADR link https://www.notion.so/dymension/ADR-x-Sequencer-Reward-Address-On-Rollapp-da84af6888c141d0a4c1a8df256a5025
Closes dymensionxyz/dymension#1248


For Author:

  • Targeted PR against correct branch
  • included the correct type prefix in the PR title
  • Linked to Github issue with discussion and accepted design
  • Targets only one github issue
  • Wrote unit and integration tests
  • All CI checks have passed
  • Added relevant godoc comments

For Reviewer:

  • confirmed the correct type prefix in the PR title
  • Reviewers assigned
  • confirmed all author checklist items have been addressed

After reviewer approval:

  • In case targets main branch, PR should be squashed and merged.
  • In case PR targets a release branch, PR should be rebased.

@keruch keruch changed the base branch from main to feat/consensus-messages October 7, 2024 17:12
@keruch keruch changed the title Kirill/1248 sequencer reward addr feat(executor): added MsgUpsertSequencer consensus message Oct 7, 2024
@keruch keruch marked this pull request as ready for review October 7, 2024 17:13
@keruch keruch requested a review from a team as a code owner October 7, 2024 17:13
Base automatically changed from feat/consensus-messages to main October 9, 2024 13:04
faultytolly
faultytolly previously approved these changes Oct 9, 2024
@faultytolly
Copy link
Contributor

I think there should be a way to test it

block/produce.go Outdated
return []proto.Message{&sequencertypes.MsgUpsertSequencer{
Operator: nextProposerSettlementAddr,
ConsPubKey: anyPubKey,
RewardAddrBytes: addrBytes,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the initial RewardAddress, the settlementAddr is used. Do we need to include both in the consensus message? Is there a scenario where this message can carry two distinct addresses, one Operator and the other Reward address?
Do I understand this correctly, to have a reward address different from the settlement addr, the custom reward address can be set on the rdk manually, in which case the RewardAddrBytes will not override it?
I realize keeping the reward address in the Hub was decided against, but if we recognize the Hub's x/sequencer record to be the source of truth for the sequencer, then we could maybe consider that besides the settlement address, the (custom) reward address could also originate from the Hub's sequencer record. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding your last point. Yes, you are right. That's what we finally decided to do with Omri. We'll enrich the rotation_started event with all the information needed for the sequencer, ie RewardAddr and WhitelistedRelayers so far. And the hub will be the source of truth.

Do we need to include both in the consensus message?

Good point, probably we don't. But as i said above, this will be change anyway soon.

Do I understand this correctly, to have a reward address different from the settlement addr, the custom reward address can be set on the rdk manually, in which case the RewardAddrBytes will not override it?

Currently, RewardAddrBytes overwrites the manually set value. Again, this will change once we accept that the Hub is the source of truce.

@keruch
Copy link
Contributor Author

keruch commented Oct 10, 2024

I think there should be a way to test it

@faultytolly Existing tests already cover the code I added. I even adjusted some existing scenarios to make tests green, you can see the diff.

@keruch
Copy link
Contributor Author

keruch commented Oct 10, 2024

The build fails while the RDK PR dymensionxyz/dymension-rdk#568 is not merged.

Copy link
Contributor

@omritoptix omritoptix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good. small code cleanliness comments.

block/produce.go Outdated
nextProposerSettlementAddr string,
lastSeqBlock bool, // Indicates that the block is the last for the current seq. True during the rotation.
) ([]proto.Message, error) {
if m.State.IsGenesis() || lastSeqBlock {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for code cleanliness we can improve it by not having all the code under the if, just return fast if it's not. (i.e if !m.state.isGenesis() && !lastSeqBlock -> return)

Copy link
Contributor

@omritoptix omritoptix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I notice now we introduce a circular dependency with the RDK which makes this pr fails to build also.

I think we should avoid the circular depedency, it makes it really difiicult to maintain after.

can't we just copy the protos?

block/produce.go Dismissed Show dismissed Hide dismissed
@keruch
Copy link
Contributor Author

keruch commented Oct 14, 2024

@omritoptix deleted a circular dependency with the RDK

@keruch keruch changed the base branch from main to kirill/1083-proto October 14, 2024 22:42
@@ -49,6 +50,7 @@ func TestClientStartup(t *testing.T) {
assert.NoError(err)
}

// TestBootstrapping TODO: this test is flaky in main. Try running it 100 times to reproduce.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add reference to #869

block/produce.go Outdated
@@ -96,18 +100,26 @@ func (m *Manager) ProduceBlockLoop(ctx context.Context, bytesProducedC chan int)
}
}

// nextProposerInfo holds information about the next proposer.
type nextProposerInfo struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this nextProposerInfo struct is uneeded, and changes alot of APIs just to be used in consensusMsgsOnCreateBlock.
nextProposerAddr can be queried using m.State.Sequencers.GetByHash (similar to SetByHash)

block/produce.go Outdated
// Currently, we need to create a sequencer in the rollapp if it doesn't exist in the following cases:
// - On the very first block after the genesis or
// - On the last block of the current sequencer (eg, during the rotation).
func (m *Manager) consensusMsgsOnCreateBlock(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i suggest to move to separate file, to have all consesnsMsg related logic

block/produce.go Outdated
tmed25519 "github.com/tendermint/tendermint/crypto/ed25519"
cmtproto "github.com/tendermint/tendermint/proto/tendermint/types"
tmtypes "github.com/tendermint/tendermint/types"
tmtime "github.com/tendermint/tendermint/types/time"

sequencertypes "github.com/dymensionxyz/dymint/types/pb/rollapp/sequencers/types"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
sequencertypes "github.com/dymensionxyz/dymint/types/pb/rollapp/sequencers/types"
rdktypes "github.com/dymensionxyz/dymint/types/pb/rollapp/sequencers/types"

block/produce.go Outdated
}

return []proto.Message{&sequencertypes.ConsensusMsgUpsertSequencer{
Operator: nextProposerSettlementAddr,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we're passing "dym1...." to the RDK, which needs to be converted to the rollapp's native prefix.
Maybe we should pass the []byte of the address, not the bech32 representation

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i had bytes here initially but i thought it wasn't convenient, so i added a method in sdk-utils to convert an arbitrary-prefixed bech32 string to AccAddress and reused it everywhere in rdk

func FromBech32[T sdk.AccAddress | sdk.ValAddress](addr string) (T, error)

dymensionxyz/sdk-utils#11

Base automatically changed from kirill/1083-proto to main October 15, 2024 10:22
@keruch keruch force-pushed the kirill/1248-sequencer-reward-addr branch from 5453a2f to 3a7aebd Compare October 16, 2024 13:54
@keruch keruch requested a review from mtsitrin October 16, 2024 13:54
@keruch keruch marked this pull request as draft October 16, 2024 18:23
@keruch keruch closed this Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sequencer Reward Address On Rollapp - Impl
5 participants